Skip to content

Fix inconsistent naming#272

Merged
jjxtra merged 9 commits intoDigitalRuby:masterfrom
superhappychris:fix-inconsistent-naming
Oct 29, 2018
Merged

Fix inconsistent naming#272
jjxtra merged 9 commits intoDigitalRuby:masterfrom
superhappychris:fix-inconsistent-naming

Conversation

@superhappychris
Copy link
Copy Markdown
Contributor

This is a pretty large refactor attempting to bring some more consistent naming to several of the model class properties. More specifically, every time a symbol is referred to in code it should be in reference to a currency pair, consisting of the following elements (order matters): a base currency, an optional separator, and a quote currency. For example, consider the symbol "BTC-USD".. If BTC-USD==20,000.00; then 1 BTC is exchanged for 20000 USD. See issue #267 for more info.

…d ExchangeMarket.MarketCurrency to ExchangeMarket.BaseCurrency. Update each implementation of OnGetSymbolsMetadataAsync to ensure they are all settings the correct properties.
…OrderDetails" and "symbols" methods to Console project.
…l, ConvertVolume to QuoteCurrency, QuoteVolume, BaseCurrency, and BaseVolume respectively. Update all usages. Add "tickers" method to Console project.
…rrencyVolume and QuoteCurrencyVolume respectively. Update all usages. Add "candles" method to Console project.
@bichuga
Copy link
Copy Markdown
Contributor

bichuga commented Oct 19, 2018

so across these tabs, are those the base currencies or the quote currencies? Somehow these names still aren't obvious to me. I thought the BTC/ETH/USDT were the bases since everything jumps off of there but from your description it seems like I have it backwards?

image

Are there names that would be so obvious no one could confuse it? Just to pull something from left field, BaseCurrency/AltCurrency? The shitcoins would always be the alts, and from the perspective of fiat to big crypto, fiat is the base and crypto is the alt.

@superhappychris
Copy link
Copy Markdown
Contributor Author

superhappychris commented Oct 19, 2018

@bichuga you have it backwards ... another way of thinking about it when you're placing an order is that the price is always denominated in the quote currency, and the quantity is always of the base currency. I know it can be confusing, especially because a couple of the exchanges even get it wrong (Bittrex 🤦‍♂️), but these are the standard terms used when trading currency pairs. Trying to come up with new names will confuse people even more I'm afraid

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 19, 2018

We can update the readme to be clear about naming conventions with whatever we decide is right.

@bichuga
Copy link
Copy Markdown
Contributor

bichuga commented Oct 19, 2018 via email

@superhappychris
Copy link
Copy Markdown
Contributor Author

@bichuga I see a couple problems with using BaseCurrency/AltCurrency...

  • For the vast majority of exchanges (I think Bittrex is the only exception), the term BaseCurrency means the opposite of how you want to use it. Obviously the path of least confusion would be to go with what the majority of exchanges do.
  • AltCurrency or AltCoin already has a pretty specific meaning to most people and it doesn't have anything to do with trading currency pairs, it has to do with the subjective quality of the coin or token in question. What you consider a shitcoin may not be considered a shitcoin by everyone else, and those definitions may change over time.
  • Honestly, the only reason we'd need to be clear about the terms BaseCurrency / QuoteCurrency is to clarify to contributors that a couple rogue exchanges use them incorrectly. Other than that, those terms would only be confusing to a person that is new to the idea of trading currency pairs in general, or that got some bad info from somewhere. Those people are going to be confused no matter what terms we use. Nothing wrong with that, I was confused at first too, everyone has to learn at some point.

I don't think we need to reinvent the wheel here. Base Currency / Quote Currency are the most widely used terms out there. The majority of exchanges use those terms.

@bichuga
Copy link
Copy Markdown
Contributor

bichuga commented Oct 19, 2018 via email

@superhappychris
Copy link
Copy Markdown
Contributor Author

@bichuga well there's only one major exchange I know of that actually got the meaning of the terms backwards, and that's Bittrex. Given the fact that Bittrex was created by a couple of developers with a background in security and not finance, it's understandable how they could have gotten it wrong, and fixing a mistake like that would be very costly. I'm trying to help us avoid making the same mistake 😄

I guess we'll just have to agree to disagree that BaseCurrency / QuoteCurrency are good names. Not only do they provide users the information they need to place an order for that trading pair (price in quote currency, quantity in base currency), but they are also the industry standard terms which means using them will lead to less confusion overall.

@vslee
Copy link
Copy Markdown
Collaborator

vslee commented Oct 20, 2018

I like BaseCurrency / QuoteCurrency because this will also align with other APIs as well as when viewing the currencies on the websites of the exchanges. Inventing new terms would just make it more difficult for the users to have to learn new concepts.
I do agree that BaseCurrency / QuoteCurrency are not very clear in their meaning and it is unfortunate that they got picked, but now that it is an industry standard we should follow it.

@bichuga
Copy link
Copy Markdown
Contributor

bichuga commented Oct 21, 2018 via email

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 21, 2018

Casting my vote for base / quote currency

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

So I see everywhere that symbol is called currency, but to me that means a single asset, like BTC. I think we should consider renaming all the places where symbol was used to market or currencyPair. Thoughts?

3 similar comments
@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

So I see everywhere that symbol is called currency, but to me that means a single asset, like BTC. I think we should consider renaming all the places where symbol was used to market or currencyPair. Thoughts?

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

So I see everywhere that symbol is called currency, but to me that means a single asset, like BTC. I think we should consider renaming all the places where symbol was used to market or currencyPair. Thoughts?

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

So I see everywhere that symbol is called currency, but to me that means a single asset, like BTC. I think we should consider renaming all the places where symbol was used to market or currencyPair. Thoughts?

@superhappychris
Copy link
Copy Markdown
Contributor Author

@jjxtra if you take a look at how the exchanges use "symbol" in their apis, they all use it in the currency pair sense, not the single asset sense, which is why I went with the former when refactoring. To me, this is another case of why come up with a new term when one already exists that is used by the exchanges?

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

Right. Maybe we should rename the new "currency" parameters to "currencyPair" parameters where they used to be "symbol" parameters.

@superhappychris
Copy link
Copy Markdown
Contributor Author

@jjxtra I think maybe I'm misunderstanding what you're looking to change.. Can you point to an example in the code so I can get on the same page as you?

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 22, 2018

Nevermind, it looks like the methods that take currency/market pairs are still using the "symbol" parameter name, such as GetTickerAsync.

@bichuga
Copy link
Copy Markdown
Contributor

bichuga commented Oct 23, 2018 via email

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 23, 2018

I vote for market.

@vslee
Copy link
Copy Markdown
Collaborator

vslee commented Oct 23, 2018

Market or CurrencyPair are good. Symbol is vague but is used in some exchanges, but doesn't seem like it would hurt to avoid it. Perhaps Symbol can be placed in the doc comments so that it does appear if ppl mouse over Market/CurrencyPair.

@superhappychris
Copy link
Copy Markdown
Contributor Author

superhappychris commented Oct 23, 2018

I think market is too vague, especially since we already have a class called ExchangeMarket. If we go with market, then the ExchangeMarket class will have a property called Market that is of type string, but there will be other places in the code base that refer to a "market" as an ExchangeMarket type...

What if we go with "MarketSymbol"? It's unambiguous, more in line with what the many of the exchanges use terminology wise, and lends itself to future changes if/when we decide to create a MarketSymbol type to encapsulate the metadata that can't be captured in a string (individual currency names, separator character, order, etc.). Thoughts?

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 23, 2018

Sounds reasonable. How do you feel about renaming all the symbol params in the pull request? Hopefully just a case sensitive search and replace of entire word.

@superhappychris
Copy link
Copy Markdown
Contributor Author

@jjxtra I can take care of it 👍

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 23, 2018

If we do use a MarketSymbol class in the future it's easy enough to make an implicit string operator conversion, so people can still pass string parameter for this.

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 24, 2018

I did some code edits on my iPad, but obviously wasn't able to ensure they compile, let me know if you see any issues.

@superhappychris
Copy link
Copy Markdown
Contributor Author

Ok it builds again

@jjxtra
Copy link
Copy Markdown
Collaborator

jjxtra commented Oct 28, 2018

Anyone have any objection to merging this?

@vslee
Copy link
Copy Markdown
Collaborator

vslee commented Oct 29, 2018

Go for it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants